Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refactor(payments): Update Checkout component to utilize Tailwind #14000

Merged
merged 1 commit into from
Aug 31, 2022
Merged

refactor(payments): Update Checkout component to utilize Tailwind #14000

merged 1 commit into from
Aug 31, 2022

Conversation

xlisachan
Copy link
Contributor

@xlisachan xlisachan commented Aug 24, 2022 •
edited
Loading

Because

  • we are converting to Tailwind

This pull request

  • Replaces all CSS/SCSS with Tailwind utility classes
  • Removes the style from .scss file with the exception of:
    • responsive design which will be addressed in a separate ticket
    • pay-with-heading's &:before and &:after
  • Reworks Storybook files

Additional components

  • PaypalButton - Moved outer div's className into component
  • PaymentMethodHeader - Updated to utilized shared class step-header
  • PlanDetails - Revised color of coupon info logo and text to a darker green for better color contrast against its background
  • SubscriptionTitle - Moved text-align to its own line

Checklist

Put an x in the boxes that apply

  • My commit is GPG signed.
  • If applicable, I have modified or added tests which pass locally.

Screenshots (Optional)

Before / After
Screen Shot 2022-08-24 at 10 22 48 AM

xlisachan
xlisachan commented Aug 24, 2022
View reviewed changes
packages/fxa-payments-server/src/routes/Checkout/index.tsx
showFxaLegalFooterLinks={true}
/>
}

<PaymentProcessing
provider="paypal"
className={classNames({
hidden: !transactionInProgress || subscriptionError,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Investigating - I tried to convert this like the others, but the test would fail. 🤨

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looked into this. Keep it. Bringing part of our Slack discussion here, more work needs to be done and a follow up ticket to PaymentProcessing, which includes other components (e.g. SubscriptionTitle and PaymentLegalBlurb). It currently fails DRY and needs a few updates made to its tests and what is expected to be/not to be in the document.

xlisachan
xlisachan commented Aug 24, 2022
View reviewed changes
packages/fxa-payments-server/src/routes/Checkout/index.scss Outdated
grid-template-columns: minmax(20px, 1fr) auto minmax(20px, 1fr);
grid-gap: 20px;
&:before,
Copy link
Contributor Author

@xlisachan xlisachan Aug 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Investigating - When I converted this to Tailwind the border appeared thicker

Edit: Figured out grid-template-columns, still stumped by the border thickness.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keep for now. Borders have to be updated for all components and later subscriptions. Either a new ticket will be filed or added to existing ticket regarding TW global/shared styles.

xlisachan
xlisachan commented Aug 24, 2022
View reviewed changes
packages/fxa-payments-server/src/routes/Product/SubscriptionCreate/index.tsx Outdated
? null
: <SubscriptionTitle screenType="create" />
}

<div
Copy link
Contributor Author

@xlisachan xlisachan Aug 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Investigating (though not a blocker as we can handle this in its ticket) - when I tried converting this to remove classNames, the test would fail.

Edit: Should I take this out since we will be filing a ticket to update SubscriptionCreate anyways?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed that this isn't a blocker. Keep for now as a fallback and add a note to the ticket (and/or a comment above) to remove once SubscriptionCreate has been updated.

@sardesam sardesam self-assigned this Aug 29, 2022
@xlisachan xlisachan marked this pull request as ready for review August 29, 2022 22:43
@xlisachan xlisachan requested a review from a team as a code owner August 29, 2022 22:43
@xlisachan xlisachan requested a review from sardesam August 29, 2022 22:43
@xlisachan xlisachan force-pushed the FXA-5572_Checkout branch 5 times, most recently from 7d7047e to bd215e5 Compare August 30, 2022 22:46
@xlisachan xlisachan force-pushed the FXA-5572_Checkout branch 2 times, most recently from 9ea4d84 to 3fead0b Compare August 30, 2022 23:35
sardesam
sardesam approved these changes Aug 31, 2022
View reviewed changes
Copy link
Contributor

@sardesam sardesam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r+ 👍 .

See individual comments.

packages/fxa-payments-server/src/routes/Checkout/index.tsx
showFxaLegalFooterLinks={true}
/>
}

<PaymentProcessing
provider="paypal"
className={classNames({
hidden: !transactionInProgress || subscriptionError,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looked into this. Keep it. Bringing part of our Slack discussion here, more work needs to be done and a follow up ticket to PaymentProcessing, which includes other components (e.g. SubscriptionTitle and PaymentLegalBlurb). It currently fails DRY and needs a few updates made to its tests and what is expected to be/not to be in the document.

packages/fxa-payments-server/src/routes/Checkout/index.scss Outdated
grid-template-columns: minmax(20px, 1fr) auto minmax(20px, 1fr);
grid-gap: 20px;
&:before,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keep for now. Borders have to be updated for all components and later subscriptions. Either a new ticket will be filed or added to existing ticket regarding TW global/shared styles.

packages/fxa-payments-server/src/routes/Product/SubscriptionCreate/index.tsx Outdated
? null
: <SubscriptionTitle screenType="create" />
}

<div
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed that this isn't a blocker. Keep for now as a fallback and add a note to the ticket (and/or a comment above) to remove once SubscriptionCreate has been updated.

@xlisachan xlisachan merged commit 1e6350a into main Aug 31, 2022
@xlisachan xlisachan deleted the FXA-5572_Checkout branch August 31, 2022 16:38
@xlisachan xlisachan mentioned this pull request Sep 6, 2022
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pdehaan pdehaan pdehaan left review comments

@sardesam sardesam sardesam approved these changes

Assignees

@sardesam sardesam

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@xlisachan @pdehaan @sardesam